Skip to content

Conversation

@machinekoder
Copy link
Contributor

src/api.js Outdated
if (S.token.value == "property" && (S.token.type == "name" || S.token.value == "var")) {
next();
return qmlpropdef();
if (!is_token(peek(), "punc", ":")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Linter didn't catch this for some reason, but there is no need for nested ifs here, just use a single one:

      if (S.token.value == "property" &&
          (S.token.type == "name" || S.token.value == "var") &&
          !is_token(peek(), "punc", ":")) {
        next();
        return qmlpropdef();
      }

Upd: ah, we don't have linter enabled in this repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, S.token.value == "property" && (S.token.type == "name" || S.token.value == "var") could be reduced to S.token.value == "property" && S.token.type == "name", perhaps this means that there is some other bug somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, token.value should never be property and var at the same time.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 23, 2016

LGTM with a nit (unnecessary if nesting).

Thanks!

@ChALkeR ChALkeR merged commit cb81dbe into qmlweb:master Oct 24, 2016
ChALkeR added a commit that referenced this pull request Oct 24, 2016
PR-URL: #23
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Oct 24, 2016
PR-URL: #23
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented Oct 24, 2016

Thanks, landed in dfbcf70!

There was some more dead code, so I removed all those in a separate commit (eba88a9), and then rebased the actual change from this PR manually.

@machinekoder machinekoder deleted the propertyfix branch October 24, 2016 19:22
@machinekoder
Copy link
Contributor Author

Awesome! When will you publish the next release?

@ChALkeR
Copy link
Member

ChALkeR commented Oct 24, 2016

@machinekoder
qmlweb-parser — just published 0.3.1.
qmlweb — not sure yet, I was hoping to finish some more structural changes before 0.2.0, but I might postpone those and release 0.2.0 pretty soon instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants